Skip to content

feat: add resource fetcher to CachingInboundEventSource #1428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Sep 5, 2022
Merged

feat: add resource fetcher to CachingInboundEventSource #1428

merged 2 commits into from
Sep 5, 2022

Conversation

scrocquesel
Copy link
Contributor

@scrocquesel scrocquesel commented Aug 28, 2022 •
edited
Loading

Refs: #1424

Signature change but I doubt anyone use this event source as it is not currently working well.

Expiration needs a single fetcher by secondary, So I prefered seperating the behavior in a another event source as not everyone will need this.

csviri
csviri requested changes Aug 29, 2022
View reviewed changes
Copy link
Collaborator

@csviri csviri left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
The implementation looks good to me, could you add unit tests pls?

@csviri
Copy link
Collaborator

csviri commented Aug 29, 2022

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)

This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted. But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

@scrocquesel
Copy link
Contributor Author

Maybe take a look on:

https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)

This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

@scrocquesel
Copy link
Contributor Author

Thank you! The implementation looks good to me, could you add unit tests pls?

I know you will ask for but as there wasn't yet, I secretly hope you will not :)
I think it also miss an integration test to show how to use it. Should the class be derived and start/stop overriden to start/stop a web listener and bind request to the public handle mthod. Or should it be share somehow between the operator and a web listener. I'm especially wondering how it could be integrated in a quarkus operator with quarkus rest services or websocket.

@csviri
Copy link
Collaborator

csviri commented Aug 30, 2022

Or should it be share somehow between the operator and a web listener.

It should, I think operator should not deal at this point with servlets.

@csviri
Copy link
Collaborator

csviri commented Aug 30, 2022 •
edited
Loading

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)
This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

Ahh nice.
Shoudn't we fetch the resource for onResourceCreated (async way, so the Thread is not blocked) ? So the reconciliation is faster.

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

Ok, agree, let's stay with the simple approach for now.

@scrocquesel
Copy link
Contributor Author

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)
This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

Ahh nice. Shoudn't we fetch the resource for onResourceCreated (async way, so the Thread is not blocked) ? So the reconciliation is faster.

It will be fetched by the getSecondaryResources ? What would be the race condition in this case ? Will it wait for the parrallel fetch to complete ?

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

Ok, agree, let's stay with the simple approach for now.

scrocquesel
scrocquesel commented Aug 30, 2022
View reviewed changes
.../javaoperatorsdk/operator/processing/event/source/inbound/CachingInboundEventSourceTest.java
verify(supplier, times(1)).fetchResources(eq(testCustomResource));
verify(eventHandler, never()).handleEvent(any());

source.handleResourceEvent(ResourceID.fromResource(testCustomResource),
Copy link
Contributor Author

@scrocquesel scrocquesel Aug 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@csviri Had to use the Set.of overload to avoid replacing the existing secondary. I guess the correct behavior should be additive.

source.handleResourceEvent(primaryID, a)
source.handleResourceEvent(primaryID, b)
source.handleResourceEvent(primaryID, c)
source.handleResourceDeleteEvent(primaryID, c)
source.getSecondaryResources(testCustomResource) // should return (a, b)
source.handleResourceEvent(primaryID, Set.of(x,y))
source.getSecondaryResources(testCustomResource) // should return (x,y)

Wait for your confirmation and will add javadoc so it's more clear to the user

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it should not be. At least that is not what I had on my mind.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably that would be good to document.
We could add overloaded or rather differerently named versions for additiveness, that would be useful for sure. But this is on purpose, now.

csviri
csviri approved these changes Aug 31, 2022
View reviewed changes
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@csviri csviri linked an issue Aug 31, 2022 that may be closed by this pull request
Improvements on CachingInboundEventSource #1424
Closed
@csviri csviri requested a review from metacosm September 2, 2022 06:50
@csviri
Copy link
Collaborator

csviri commented Sep 2, 2022

@metacosm this LGTM, pls take a look if you will find some time.

@csviri
Copy link
Collaborator

csviri commented Sep 2, 2022

@scrocquesel could you rebase this to the next branch? and target next with the PR. next contains the next minor release with new features, main is usually there for patching.

@csviri csviri merged commit f46db1b into operator-framework:main Sep 5, 2022
@scrocquesel scrocquesel deleted the gh-1424 branch October 15, 2022 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metacosm metacosm metacosm approved these changes

@csviri csviri csviri approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improvements on CachingInboundEventSource
3 participants
@scrocquesel @csviri @metacosm